WPB-20214: Add member searchability #4786

eyeinsky · 2025-09-23T11:27:06Z

Task checklist:

Open questions

is the changed admin toggle API type ok? The task designates this as POST /users/:uid/searchable, but as TeamId is required, then it currently is POST /users/:uid/:tid/searchable -- could this be improved?
POST /handles: should this endpoint also filter based on searchability?
are there no other locations than /search/contacts to take searchability into account?
should the test be moved into the integration package? (@battermann mentioned this -- are we moving tests there and hence, should this test also go there as well?)

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

eyeinsky · 2025-09-25T12:49:49Z

The below is now resolved.

The current error when running the test is this:

[[email protected]] E, IO Exception occurred, message=cql-io: protocol error: parse error: response body reading: Failed reading: column count: 24 =/= 26 Empty call stack , request=37d3c1fa-f1ee-4050-b623-e4c9c79e5584
[[email protected]] E, request=37d3c1fa-f1ee-4050-b623-e4c9c79e5584, code=500, label=server-error, "Server Error"
brig-integration: Assertions failed:
 1: 201 =/= 500

Response was:

Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Thu, 25 Sep 2025 09:00:01 GMT"),("Server","Warp/3.4.2"),("traceparent","00-96836cd3b1eedf0553338dde6b2d4b0c-1ced0252881c377c-01"),("tracestate",""),("Content-Encoding","gzip"),("Content-Type","application/json"),("Vary","Accept-Encoding")], responseBody = Just "{\"code\":500,\"label\":\"server-error\",\"message\":\"Internal Server Error\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose, responseOriginalRequest = Request {
  host                 = "127.0.0.1"
  port                 = 8082
  secure               = False
  requestHeaders       = [("Content-Type","application/json")]
  path                 = "/i/users"
  queryString          = ""
  method               = "POST"
  proxy                = Nothing
  rawBody              = False
  redirectCount        = 10
  responseTimeout      = ResponseTimeoutDefault
  requestVersion       = HTTP/1.1
  proxySecureMode      = ProxySecureWithConnect
}
, responseEarlyHints = []}
CallStack (from HasCallStack):
  error, called at src/Bilge/Assert.hs:91:5 in bilge-0.22.0-inplace:Bilge.Assert
  <!!, called at test/integration/API/Team/Util.hs:140:9 in brig-2.0-inplace-brig-integration:API.Team.Util
  createUserWithTeam', called at test/integration/API/Team/Util.hs:89:21 in brig-2.0-inplace-brig-integration:API.Team.Util
  createPopulatedBindingTeamWithNames, called at test/integration/API/Team/Util.hs:68:25 in brig-2.0-inplace-brig-integration:API.Team.Util
  createPopulatedBindingTeamWithNamesAndHandles, called at test/integration/API/Search.hs:165:38 in brig-2.0-inplace-brig-integration:API.Search

From running this command: make c package=all && ./hack/bin/cabal-run-integration.sh brig -p '/testUserSearchable/'

eyeinsky · 2025-10-06T10:18:02Z

@akshaymankar Noting here what you asked about in the standup: the /search/contacts endpoint fetches the results from ES which is populated from Cassandra's user table. This has the searchable field, so filtering there is easy. The /team/:tid/members?searchable=false on the other hand gets its results from Cassandra's team_member table, which doesn't have a searchable field.. But since I need to return all non-searchable users from this table, then I think the current (pre-postgres) way to do it would be to get all rows from team_member and then filter that down to only non-searchable users by the help of users table. With large table this might costly to do. 🤔

When we had all data in postgres though, the query could entirely be done on the database side: join team_member with user, then filter by searchable.

akshaymankar · 2025-10-06T11:23:49Z

@eyeinsky I think we shouldn't support this query param on /teams/:tid/members endpoint, but rather on /teams/:tid/search endpoint. I just saw the ticket and it seems like I made a mistake in the name of this endpoint. Sorry about that 😞

eyeinsky · 2025-10-07T09:51:54Z

@akshaymankar I'm ready for another round of review!

akshaymankar · 2025-10-07T13:34:21Z

is the changed admin toggle API type ok? The task designates this as POST /users/:uid/searchable, but as TeamId is required, then it currently is POST /users/:uid/:tid/searchable -- could this be improved?

I think we can just lookup the team id of the targetted user and check whether the user who made the request is an admin of that team. We shouldn't need team id in the path.

POST /handles: should this endpoint also filter based on searchability?

No, we can expose the fact that his handle is taken as far as we don't tell who has this handle.

are there no other locations than /search/contacts to take searchability into account?

Yeah, this is the only location.

should the test be moved into the integration package? (@battermann mentioned this -- are we moving tests there and hence, should this test also go there as well?)

Ideally, yes. Sometimes we've been lazy, but the agreed upon rule was to move a test if you need to work on it and not to add more tests in the legacy test suites.

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

libs/wire-api/src/Wire/API/Team/Permission.hs

libs/wire-api/test/golden/Test/Wire/API/Golden/Generated/SearchResult_20TeamContact_user.hs

libs/wire-api/src/Wire/API/User/Search.hs

libs/wire-api/test/unit/Test/Wire/API/User.hs

services/brig/src/Brig/Provider/API.hs

services/brig/test/integration/API/User/Account.hs

libs/wire-subsystems/src/Wire/IndexedUserStore/ElasticSearch.hs

libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs

battermann

This needs a release note explaining how the search index mapping has to be updated and how to avoid downtime, etc.

And there are some minor comments, looks good overall.

battermann · 2025-10-10T09:45:39Z

integration/test/Test/Search.hs

+  let -- Helper to change user searchability.
+      setSearchable self uid searchable = do
+        req <- baseRequest self Brig Versioned $ joinHttpPath ["users", uid, "searchable"]
+        submit "POST" $ addJSON searchable req


this should go into API.Brig

battermann · 2025-10-10T09:49:36Z

integration/test/Test/Search.hs

+  u1' <- BrigP.getUser u1 u1 >>= getJSON 200
+  assertBool "Searchable is still True" =<< (u1' %. "searchable" & asBool)


If u1' is not used later I would prefer to do the assertion in a bindResponse body, to reduce number of ticked variable names. Also you could assert like this: u1 %. "searchable" shouldMatch True (I think) which I find more readable.

The above applies to the following lines, too.

battermann · 2025-10-10T09:54:42Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

+        :> QueryParam'
+             [ Optional,
+               Strict,
+               Description "Optional, return only non-seacrhable members when false."


will it return only searchable members when true?

It would have passed the variable explicitly forward to ES, but that would have been confusing (?searchable=true would have returned users where this flag is explicitly set to true), so it is now changed at the latest iteration 2ced710.

battermann · 2025-10-10T10:04:29Z

services/brig/test/integration/Util.hs

+randomUserPrefix ::
+  (MonadCatch m, MonadIO m, MonadHttp m, HasCallStack) =>
+  Text ->
+  Brig ->
+  m User
+randomUserPrefix prefix brig = do
+  n <- fromName <$> randomName
+  createUser' True (prefix <> n) brig
+


this seems unused, no?

Yup, removed!

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

eyeinsky · 2025-10-10T13:26:47Z

@battermann I've now adapted to all of the above comments in the latest state of this branch (7b3b5e5).

Edit: Ah, changelog entry incoming.

Copilot

Pull Request Overview

This PR implements member searchability functionality by adding a searchable field to users that controls their visibility in public search endpoints. When set to false, users won't be found through public search but remain accessible to team admins through dedicated endpoints.

Key changes:

Added searchable boolean field to user data structures with default value true
Implemented POST /users/:uid/searchable API endpoint with team admin permission requirements
Modified search endpoints to filter based on searchability while preserving admin access

Reviewed Changes

Copilot reviewed 31 out of 36 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
libs/wire-api/src/Wire/API/User.hs	Added `profileSearchable` and `userSearchable` fields to user data types
libs/wire-api/src/Wire/API/Team/Member.hs	Added `SetMemberSearchable` permission for team admins
libs/wire-api/src/Wire/API/Routes/Public/Brig.hs	Added API route for setting user searchability
libs/wire-subsystems/src/Wire/UserSubsystem/Interpreter.hs	Implemented searchability filtering in local search and admin endpoint
libs/wire-subsystems/src/Wire/IndexedUserStore/ElasticSearch.hs	Added Elasticsearch filtering for searchable users
services/brig/src/Brig/Data/User.hs	Updated database schema and queries to include searchable field
integration/test/Test/Search.hs	Added comprehensive integration tests for searchability feature
changelog.d/2-features/WPB-20214	Added feature changelog entry

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-10T15:05:25Z

services/brig/src/Brig/Data/User.hs

          managedBy = managedBy,
-          supportedProtocols = prots
+          supportedProtocols = prots,
+          searchable = True -- NewUser doesn't have this field. TODO: should a defSearchable be added to Wire.API.User to be used everywhere?


The TODO comment suggests adding a default constant but leaves it unresolved. Consider either implementing the suggested defSearchable constant in Wire.API.User or removing this TODO if the current approach is acceptable.

a40d240, we add it when we need it.

Copilot · 2025-10-10T15:05:26Z

libs/wire-subsystems/test/unit/Wire/UserSearch/TypesSpec.hs

      udSso = Nothing,
-      udEmailUnvalidated = Nothing
+      udEmailUnvalidated = Nothing,
+      udSearchable = Nothing -- TODO: or Just True?


The TODO comment questions whether this should be Nothing or Just True. This uncertainty in test data could indicate unclear requirements. Consider resolving this based on the expected behavior for legacy data without the searchable field.

Suggested change

udSearchable = Nothing -- TODO: or Just True?

udSearchable = Nothing

ffba78f, keeping it as Nothing as if the value didn't exist in Elastic Search.

Copilot · 2025-10-10T15:05:26Z

libs/wire-api/src/Wire/API/Team/Permission.hs

+--
+-- If you ever think about adding a new permission flag, read Note
+-- [team roles] first.


[nitpick] The comment about reading 'Note [team roles]' was moved from after the data type definition to before it, but the new SetMemberSearchable permission was added without any indication that this note was consulted. Consider adding a comment confirming this note was reviewed for the new permission.

If it was moved, it was definitely read. I moved it because it relates to the entire data type (as when it was next to a data constructor, it looked as if it only mattered to that particular one).

Copilot · 2025-10-10T15:05:27Z

libs/bilge/src/Bilge/Assert.hs

  Assertions () ->
  m ()
-(!!!) io = void . (<!!) io
+io !!! aa = void (io <!! aa)


This change from pattern matching to direct function composition appears to be unrelated to the searchability feature. Consider moving this refactoring to a separate commit or PR to maintain clear separation of concerns.

Suggested change

io !!! aa = void (io <!! aa)

m !!! a = do

_ <- m <!! a

pure ()

This is part of a functionally no-op refactor commit. The single line version is shorter, but still clear.

Copilot · 2025-10-10T15:05:27Z

libs/wire-api/src/Wire/API/Routes/Public/Brig.hs

 type UserAPI =
-  -- See Note [ephemeral user sideeffect]
  Named


The removal of 'See Note [ephemeral user sideeffect]' comments appears unrelated to the searchability feature. These documentation changes should be part of a separate commit focused on documentation cleanup.

These removals are in a separate commit.

This reverts commit 791a4cb.

eyeinsky · 2025-10-14T14:04:48Z

Right, changed it back.

eyeinsky · 2025-10-15T10:17:36Z

@akshaymankar I fixed the last issue here 5acb9f7, and also test that the result for /team/:tid/search and /team/:tid/search?searchable=true is the same here (for me it doesn't scroll to the right place for some reason, but it's highlighted).

eyeinsky · 2025-10-15T10:18:58Z

Now re-running CI after make sanitize-pr.

eyeinsky · 2025-10-15T15:45:54Z

Bumping CI, the Demo.testDynamicBackend test failed, but that seems unrelated.

akshaymankar · 2025-10-16T08:28:14Z

libs/wire-api/src/Wire/API/User/Search.hs

+  { setSearchable :: Bool
+  }
+  deriving (Generic)
+  deriving anyclass (Aeson.ToJSON, Aeson.FromJSON, S.ToSchema)


This would create camelCase JSON fields. We use snake_case everywhere. Can you please use ToSchema from Data.Schema and then deriving ToJSON, FromJSON and S.ToSchema.

akshaymankar · 2025-10-16T08:39:08Z

integration/test/Test/Search.hs

+  -- /teams/:tid/search and /teams/:tid/search?searchable=true both get all members, searchable and non-searchable
+  noQueryParam <-
+    BrigP.searchTeam admin [] `bindResponse` \resp -> do
+      resp.status `shouldMatchInt` 200
+      docs <- resp.json %. "documents" >>= asList
+      mapM (\m -> m %. "id" & asString) docs
+  withQueryParam <-
+    BrigP.searchTeam admin [("searchable", "true")] `bindResponse` \resp -> do
+      resp.status `shouldMatchInt` 200
+      docs <- resp.json %. "documents" >>= asList
+      mapM (\m -> m %. "id" & asString) docs
+  assertBool "/teams/:tid/search and /teams/:tid/search?searchable=true are equal"
+    $ Set.fromList noQueryParam
+    == Set.fromList withQueryParam


These are not the same cases. When searchable=true we return only searchable people, But when it is not set we should return all users. Otherwise there is no way for the admin to see all the users.

make sanitize-pr

fix typos

one more typo

yet another

eyeinsky · 2025-10-20T07:14:20Z

@akshaymankar I've now hopefully implemented the correct behavior to /teams/:tid/search!

Also, on Friday I added a test for the legacy situation of where searchable field is missing from ES here to services/brig/test/integration/API/Search.hs (there, because ES config is made accessible there from the test setup).

akshaymankar

Looks good, thanks!

@battermann

@battermann is on vacation, I've reviewed the work now.

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 23, 2025

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch 5 times, most recently from bc31287 to 78ad65e Compare September 25, 2025 08:44

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch 3 times, most recently from 9891c56 to 6a1a1b8 Compare October 3, 2025 07:56

eyeinsky marked this pull request as ready for review October 3, 2025 08:17

eyeinsky requested a review from a team as a code owner October 3, 2025 08:17

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 6a1a1b8 to 4790b05 Compare October 6, 2025 09:59

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 4790b05 to b0f782c Compare October 7, 2025 09:47

akshaymankar requested changes Oct 7, 2025

View reviewed changes

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch 3 times, most recently from c0e5357 to cfc7888 Compare October 10, 2025 09:27

battermann previously requested changes Oct 10, 2025

View reviewed changes

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch 2 times, most recently from 833088a to d8fb31c Compare October 10, 2025 12:40

eyeinsky requested review from a team as code owners October 10, 2025 13:34

battermann requested a review from Copilot October 10, 2025 15:03

Copilot AI reviewed Oct 10, 2025

View reviewed changes

Revert "Pass explicit field value down to ES; adapt test"

5acb9f7

This reverts commit 791a4cb.

make sanitize-pr

1233dc0

eyeinsky added 3 commits October 15, 2025 15:39

fix templates

8de6015

add seacrhable to golden tests

cfd7a91

Implement searchable flag to MockInterpreters

8e395fa

eyeinsky force-pushed the ml/WPB-20214-user-searchable branch from 329316d to 8e395fa Compare October 15, 2025 14:25

bump CI

7195d36

fixup! bump CI

28447bc

akshaymankar requested changes Oct 16, 2025

View reviewed changes

eyeinsky added 11 commits October 16, 2025 16:00

second attempt at correct /team/:tid/search

25fa40f

fixup! second attempt at correct /team/:tid/search

18cc561

Use Snake case object field for SetSearchable

6982181

fixup! second attempt at correct /team/:tid/search

bd04a10

make sanitize-pr

fixup! Use Snake case object field for SetSearchable

0ea4a7c

Add test to check that legacy users are found

8d46700

fixup! Add test to check that legacy users are found

8a74d63

fix typos

fixup! Add test to check that legacy users are found

7879f1c

one more typo

fixup! Add test to check that legacy users are found

cfc9caf

yet another

make sanitize-pr

fe13075

fixup! make sanitize-pr

1646b92

akshaymankar approved these changes Oct 20, 2025

View reviewed changes

eyeinsky merged commit b51531c into develop Oct 20, 2025
9 checks passed

eyeinsky deleted the ml/WPB-20214-user-searchable branch October 20, 2025 08:46

zebot mentioned this pull request Oct 20, 2025

Release 2025-10-20 - (expected chart version 5.23.0) #4818

Draft

		u1' <- BrigP.getUser u1 u1 >>= getJSON 200
		assertBool "Searchable is still True" =<< (u1' %. "searchable" & asBool)

	udSearchable = Nothing -- TODO: or Just True?
	udSearchable = Nothing

-io !!! aa = void (io <!! aa)
+m !!! a = do
+  _ <- m <!! a
+  pure ()

WPB-20214: Add member searchability #4786

WPB-20214: Add member searchability #4786

Uh oh!

Conversation

eyeinsky commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Open questions

Checklist

Uh oh!

eyeinsky commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyeinsky commented Oct 6, 2025

Uh oh!

akshaymankar commented Oct 6, 2025

Uh oh!

eyeinsky commented Oct 7, 2025

Uh oh!

akshaymankar commented Oct 7, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

battermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eyeinsky commented Oct 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyeinsky commented Sep 23, 2025 •

edited

Loading

eyeinsky commented Sep 25, 2025 •

edited

Loading

eyeinsky commented Oct 10, 2025 •

edited

Loading